Skip to content

Refactor reconcilers and implementations#250

Merged
pleshakov merged 13 commits into
mainfrom
chore/refactor-implementations-and-controllers
Oct 14, 2022
Merged

Refactor reconcilers and implementations#250
pleshakov merged 13 commits into
mainfrom
chore/refactor-implementations-and-controllers

Conversation

@pleshakov
Copy link
Copy Markdown
Contributor

@pleshakov pleshakov commented Sep 23, 2022

Previously, all reconcilers and implementations looked almost the same.

This PR:

  • Replaces all reconcilers with a single one.
  • Removes all implementations. Now the reconciler sends upsert and delete events to the event channel instead of the implementations. The sending to the events part of the reconciler doesn't have the shutdown bug (a possibility to hang while sending to the channel during the shutdown) all implementations shared.
  • Removes the sdk package.

Additionally, the Manager now uses the same logger as the other components.

@github-actions github-actions Bot added the chore Pull requests for routine tasks label Sep 23, 2022
@pleshakov pleshakov added the bug Something isn't working label Sep 23, 2022
@pleshakov pleshakov force-pushed the chore/refactor-implementations-and-controllers branch from 05f549b to bd92697 Compare September 23, 2022 23:06
@github-actions github-actions Bot added dependencies Pull requests that update a dependency file and removed bug Something isn't working labels Sep 23, 2022
@pleshakov pleshakov force-pushed the chore/refactor-implementations-and-controllers branch from 966c5f4 to c6e289f Compare September 26, 2022 15:26
@pleshakov pleshakov added the bug Something isn't working label Sep 26, 2022
kate-osborn
kate-osborn suggested changes Sep 27, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is 🔥

Really nice work! This is a huge improvement over what we had before.

Just a couple of small comments and questions.

Comment thread internal/controller/controller_suit_test.go Outdated
Comment thread internal/controller/reconciler.go Outdated
Comment thread internal/controller/reconciler_test.go Outdated
Comment thread internal/controller/reconciler_test.go Outdated
Comment thread internal/controller/reconciler_test.go Outdated
Comment thread internal/controller/reconciler_test.go Outdated
Comment thread internal/controller/register.go Outdated
@github-actions github-actions Bot removed the bug Something isn't working label Sep 28, 2022
kate-osborn
kate-osborn reviewed Sep 29, 2022
View reviewed changes
Comment thread internal/controller/register.go Outdated
@pleshakov pleshakov force-pushed the chore/refactor-implementations-and-controllers branch 4 times, most recently from a594e1a to 6a252c1 Compare October 5, 2022 18:17
@pleshakov
Copy link
Copy Markdown
Contributor Author

pleshakov commented Oct 5, 2022

Changes from the previous version:

  • reconciler.Reconciler was renamed to reconciler.Implementation
  • changes from Deduplicate endpoints #253 were incorporated into the refactor -- manager package now includes index package (for indices) and predicate (for predicates)
  • tests were added to test controller registration

@pleshakov pleshakov requested a review from kate-osborn October 5, 2022 18:25
@pleshakov pleshakov added the bug Something isn't working label Oct 5, 2022
kate-osborn
kate-osborn suggested changes Oct 6, 2022
View reviewed changes
Comment thread internal/manager/controllers.go Outdated
Comment thread internal/manager/controllers.go Outdated
Comment thread internal/manager/controllers.go Outdated
Comment thread internal/manager/controllers.go Outdated
Comment thread internal/manager/controllers.go Outdated
Comment thread internal/manager/controllers.go Outdated
Comment thread internal/manager/controllers_test.go Outdated
Comment thread internal/manager/manager.go Outdated
Comment thread internal/manager/manager.go Outdated
@pleshakov pleshakov force-pushed the chore/refactor-implementations-and-controllers branch from 6a252c1 to 2d818d6 Compare October 7, 2022 22:15
@pleshakov pleshakov requested a review from kate-osborn October 7, 2022 22:16
@github-actions github-actions Bot removed the bug Something isn't working label Oct 7, 2022
kate-osborn
kate-osborn reviewed Oct 10, 2022
View reviewed changes
Comment thread internal/manager/controllers.go Outdated
Comment thread internal/manager/controllers.go Outdated
Comment thread internal/manager/index/endpointslice.go
@pleshakov pleshakov requested a review from kate-osborn October 12, 2022 00:23
kate-osborn
kate-osborn approved these changes Oct 12, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@kate-osborn kate-osborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@pleshakov pleshakov added the bug Something isn't working label Oct 12, 2022
f5yacobucci
f5yacobucci reviewed Oct 12, 2022
View reviewed changes
Comment thread internal/reconciler/implementation.go
@github-actions github-actions Bot removed the bug Something isn't working label Oct 14, 2022
@pleshakov
Refactor reconcilers and implementations
e5b21b0 
Previously, all reconcilers and implementations looked almost the same.

This commit:
- Replaces all reconcilers with a single one.
- Removes all implementations. Now the reconciler sends upsert and
delete events to the event channel instead of the implementations.
The sending to the events part of the reconciler doesn't have
the shutdown bug (a possibility to hang while sending to the channel
during the shutdown) all implementations shared.
- Removes the sdk package.

Additionally, the Manager now uses the same logger as the other
components.
@pleshakov
Update deps
1b3cba0
@pleshakov
rename extractValue to indexFunc
1ef8263
@pleshakov
Rename prepareIndex to addIndex
4c0626f
@pleshakov
Simplify error message
4cbf962
@pleshakov
Add the filter package
67097d6
@pleshakov
Rename k8sEventFilter to k8sPredicate
71fea1f
@pleshakov
Refactor EndpointSliceFieldIndexes
b2bfe8d
@pleshakov
Clarify
13dae5b
@pleshakov
Add a comment
0a67e50
@pleshakov
Introduce options
9718f69
@pleshakov
Clarify why not deep copy
d1cf071
@pleshakov pleshakov force-pushed the chore/refactor-implementations-and-controllers branch from 53b9cb8 to d1cf071 Compare October 14, 2022 16:55
@pleshakov pleshakov added the bug Something isn't working label Oct 14, 2022
@pleshakov pleshakov merged commit 7328fee into main Oct 14, 2022
@pleshakov pleshakov deleted the chore/refactor-implementations-and-controllers branch October 14, 2022 18:28
miledxz added a commit to miledxz/nginx-gateway-fabric that referenced this pull request Jan 14, 2025
@miledxz
Refactor reconcilers and implementations (nginx#250)
ec53518 
Previously, all reconcilers and implementations looked almost the same.

This commit:
- Replaces all reconcilers with a single one.
- Removes all implementations. Now the reconciler sends upsert and
delete events to the event channel instead of the implementations.
The sending to the events part of the reconciler doesn't have
the shutdown bug (a possibility to hang while sending to the channel
during the shutdown) all implementations shared.
- Removes the sdk package.

Additionally, the Manager now uses the same logger as the other
components.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@f5yacobucci f5yacobucci f5yacobucci left review comments

@kate-osborn kate-osborn kate-osborn approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bug Something isn't working chore Pull requests for routine tasks dependencies Pull requests that update a dependency file

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@pleshakov @f5yacobucci @kate-osborn